Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed flaky hybrid collector test #984

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Fixed flaky hybrid collector test #984

merged 2 commits into from
Nov 13, 2024

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented Nov 13, 2024 •
edited
Loading

Description

Fixed flakiness of testReduceWithConcurrentSegmentSearch_whenMultipleCollectorsMatchedDocs_thenSuccessful

Fix:

  1. Ensured consistent document count: Instead of using RandomizedTest.randomInt() for document IDs, use fixed values or a sequence to ensure a consistent number of documents.

  2. Control the number of documents: Add a fixed number of documents to each index, ensuring it's always greater than the number expecting in the results.

  3. Adjust the search size: Made sure the search size (when(searchContext.size()).thenReturn(1)) is consistent with the number of documents adding and expecting in the results.

Ran test 100 times, worked fine with the change

 ./gradlew ':test' --tests "org.opensearch.neuralsearch.search.query.HybridCollectorManagerTests.testReduceWithConcurrentSegmentSearch_whenMultipleCollectorsMatchedDocs_thenSuccessful" -Dtests.seed=8305B866B5D072CE -Dtests.security.manager=false -Dtests.locale=af-ZA -Dtests.timezone=Europe/Uzhgorod -Dtests.iters=100
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 8.9
  OS Info               : Mac OS X 14.7 (aarch64)
  JDK Version           : 21 (Oracle JDK)
  JAVA_HOME             : /Library/Java/JavaVirtualMachines/jdk-21.jdk/Contents/Home
  Random Testing Seed   : 8305B866B5D072CE
  In FIPS 140 mode      : false
=======================================

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.9/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD SUCCESSFUL in 1s

Related Issues

Resolves #819

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@martin-gaievski
Copy link
Member

Can you describe at high level what was the issue and what's the leading idea for this fix?

@martin-gaievski
Copy link
Member

martin-gaievski commented Nov 13, 2024 •
edited
Loading

Does the 2.x branch have same problem? If so we need to add backport 2.x label

@owaiskazi19
Copy link
Member Author

Does the 2.x branch have same problem? If so we need to add backport 2.x label

Yes, I see failure on 2.x. We should backport this one

org.opensearch.neuralsearch.search.query.HybridCollectorManagerTests > testReduceWithConcurrentSegmentSearch_whenMultipleCollectorsMatchedDocs_thenSuccessful {seed=[8305B866B5D072CE:E498F451C7B14F98]} FAILED
    java.lang.IllegalArgumentException: cannot merge top docs because it does not have enough elements
        at __randomizedtesting.SeedInfo.seed([8305B866B5D072CE:E498F451C7B14F98]:0)
        at org.opensearch.neuralsearch.search.query.HybridQueryScoreDocsMerger.merge(HybridQueryScoreDocsMerger.java:42)
        at org.opensearch.neuralsearch.search.query.TopDocsMerger.getMergedScoreDocs(TopDocsMerger.java:130)
        at org.opensearch.neuralsearch.search.query.TopDocsMerger.merge(TopDocsMerger.java:69)
        at org.opensearch.neuralsearch.search.query.HybridCollectorManager.reduceCollectorResults(HybridCollectorManager.java:446)
        at org.opensearch.neuralsearch.search.query.HybridCollectorManager.lambda$getSearchResults$0(HybridCollectorManager.java:169)
        at org.opensearch.neuralsearch.search.query.HybridCollectorManager.lambda$reduceSearchResults$11(HybridCollectorManager.java:459)
        at org.opensearch.neuralsearch.search.query.HybridCollectorManagerTests.testReduceWithConcurrentSegmentSearch_whenMultipleCollectorsMatchedDocs_thenSuccessful(HybridCollectorManagerTests.java:598)

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented Nov 13, 2024 •
edited
Loading

Can you describe at high level what was the issue and what's the leading idea for this fix?

Updated the description

@martin-gaievski martin-gaievski added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Nov 13, 2024
@martin-gaievski
Copy link
Member

Looks good to me, thank you

@vibrantvarun vibrantvarun merged commit 9412884 into opensearch-project:main Nov 13, 2024
43 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 13, 2024
@owaiskazi19 @github-actions
Fixed flaky hybrid collector test (#984)
5914697 
* Fixed flaky hybrid collector test

Signed-off-by: Owais <[email protected]>

* Removed explicit exception

Signed-off-by: Owais <[email protected]>

---------

Signed-off-by: Owais <[email protected]>
(cherry picked from commit 9412884)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 13, 2024
Merged
vibrantvarun pushed a commit that referenced this pull request Nov 13, 2024
@opensearch-trigger-bot @owaiskazi19
Fixed flaky hybrid collector test (#984) (#985)
33af4fb 
* Fixed flaky hybrid collector test

Signed-off-by: Owais <[email protected]>

* Removed explicit exception

Signed-off-by: Owais <[email protected]>

---------

Signed-off-by: Owais <[email protected]>
(cherry picked from commit 9412884)

Co-authored-by: Owais Kazi <[email protected]>
martin-gaievski pushed a commit that referenced this pull request Nov 18, 2024
@owaiskazi19 @martin-gaievski
Fixed flaky hybrid collector test (#984)
c9c3567 
* Fixed flaky hybrid collector test

Signed-off-by: Owais <[email protected]>

* Removed explicit exception

Signed-off-by: Owais <[email protected]>

---------

Signed-off-by: Owais <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-gaievski martin-gaievski martin-gaievski approved these changes

@vibrantvarun vibrantvarun vibrantvarun approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@sean-zheng-amazon sean-zheng-amazon Awaiting requested review from sean-zheng-amazon sean-zheng-amazon is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@zhichao-aws zhichao-aws Awaiting requested review from zhichao-aws zhichao-aws is a code owner

@yuye-aws yuye-aws Awaiting requested review from yuye-aws yuye-aws is a code owner

@minalsha minalsha Awaiting requested review from minalsha minalsha is a code owner

Assignees
No one assigned
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch flaky-test good first issue Good for newcomers skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Flaky test: testReduceWithConcurrentSegmentSearch_whenMultipleCollectorsMatchedDocs_thenSuccessful
3 participants
@owaiskazi19 @martin-gaievski @vibrantvarun